Add WPush Notification Provider #4603
Conversation
| "channel": notification.wpushChannel | ||
| }); | ||
| if (ret.data.code !== 0) { | ||
| return ret.data.message; |
There was a problem hiding this comment.
If this is an error condition, maybe we should do throw new Error(ret.data.message) instead.
There was a problem hiding this comment.
Overall, this looks good.
I have left some inline comments below.
I have added a table to the PR-description. Could you fill it with the requisite screnshots?
Explanation on the second one:
Offering notifications is close to the core of what we are as an uptime monitor. Therefore, making sure that they work is also really important. Because testing notification providers is quite time intensive, we mostly offload this onto the person contributing a notification provider.
To make shure you have tested the notification provider, please include screenshots of the following events in the pull-request description:
UP/DOWN- Certificate Expiry via https://expired.badssl.com/
- Testing (the test button on the notification provider setup page)
| </div> | ||
|
|
||
| <div class="mb-3"> | ||
| <label for="wpush-channel" class="form-label">发送通道</label> |
There was a problem hiding this comment.
I have not used said notification provider:
Is this option clear enough for the users?
Do they need an helptext on how to find this?
There was a problem hiding this comment.
Think , Detailed channel descriptions can be viewed at the link below
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-authored-by: Frank Elsinga <frank@elsinga.de>
CommanderStorm
added
area:notifications
There was a problem hiding this comment.
Please refer to above review. I think you missed this paragraph:
I have added a table to the PR-description. Could you fill it with the requisite screnshots?
Explanation on the second one:
Offering notifications is close to the core of what we are as an uptime monitor. Therefore, making sure that they work is also really important. Because testing notification providers is quite time intensive, we mostly offload this onto the person contributing a notification provider.
To make shure you have tested the notification provider, please include screenshots of the following events in the pull-request description:
UP/DOWN- Certificate Expiry via https://expired.badssl.com/
- Testing (the test button on the notification provider setup page)
CommanderStorm
added
pr:please address review comments
github-actions
bot
added
pr:please resolve merge conflict
github-actions
bot
added
pr:please resolve merge conflict
|
@anhao Thanks for the PR. |
|
@CommanderStorm |
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma
Tick the checkbox if you understand [x]:
Description
Add WPush Notification Provider
Type of change
Please delete any options that are not relevant.
Checklist
Screenshots (if any)
UPDOWN